Changes parseLengthCodedNumber to parse big numbers #382

dresende · 2013-01-29T21:14:56Z

The idea is to parse numbers over the JS precision using an external dependency and return the number as string.

This returns them as strings instead of numbers

dresende · 2013-01-29T21:15:38Z

Not yet to merge, just for people to comment and test locally. Documentation and tests are needed later.

kai-koch · 2013-01-29T21:53:12Z

works with the example from #380

kai-koch · 2013-01-29T22:04:42Z

If you decide at the end, the value is already overflown, must be tested around line 162:

  } else if (byte === 254) {
    length = 8;
    if (this._buffer[this._offset + 7] === 1) {
         //HELP Big Number!
    }
  } else {

felixge · 2013-01-29T22:17:38Z

lib/protocol/Parser.js

+    if (BigNumber) {
+      value = value.plus((new BigNumber(256)).pow(bytesRead).times(byte));
+    } else if (bytesRead == 7 && byte == 1) {
+      BigNumber = require("bignumber.js");


This will make blocking FS calls, even if the module has been cached before. If you want to lazy-load this, you'll need to have a top-level variable that you use as an in-module cache.

edit: never mind, I see you're doing that. ignore me

No, @felixge you were right. That variable is within the Parser.prototype.parseLengthCodedNumber function and should probably be moved out to the module level, otherwise require will be called each time a length coded number is parsed that is too large.

kai-koch · 2013-01-29T22:45:54Z

From line 156:

  var length, bytesRead;
  var BigNumber = false;
  var value = 0; 
  if (byte === 252) {
    length = 2;
  } else if (byte === 253) {
    length = 3;
  } else if (byte === 254) {
    length = 8;
    if (this._buffer[this._offset + 7] === 1) {
      BigNumber = require("bignumber.js");
      value = new BigNumber(0);
    }
  } else {
    throw new Error('parseLengthCodedNumber: Unexpected first byte: ' + byte);
  }
  for (bytesRead = 0; bytesRead < length; bytesRead++) {
    byte = this._buffer[this._offset++];
    // overflow
    if (BigNumber) {
      value = value.plus((new BigNumber(256)).pow(bytesRead).times(byte));
    } else {
      value += Math.pow(256, bytesRead) * byte;
    }
  }

  return (BigNumber ? value.toString() : value);

Works with example from #380
Also works with small numbers.
Did not run the unit tests.

dresende · 2013-01-29T22:58:55Z

@kai-koch: I like your code, I just started the pull request before optimizing because I wanted to discuss the use of that dependency. I tested others and they all seemed bloated or faulty.

@felixge : you're saying the dependency should be on top of the file? I just load it if a big number is found, otherwise never require it.. and it should cache for future requires.. no?

…bers

kai-koch · 2013-01-29T23:14:42Z

I found something strange:
If one truncates the table and then sets the the AUTO_INCREMENT like

TRUNCATE t;
ALTER TABLE  `t` AUTO_INCREMENT =72057594037927935

the first run of my example returns an unprecise JavaScript number the second one the correct number.

D:\[...]\node\wutCollector>node issue380.js
New row ID: 72057594037927940

D:\[...]\node\wutCollector>node issue380.js
New row ID: 72057594037927936

In the database the entries start correctly at 72057594037927935
Same behaviour if I drop the table and create it with the original CREATE-Statemen from #380
Any idea why?
I am on WinXP using an apache friends xampp installation with phpMyAdmin as database frontend.

dresende · 2013-01-29T23:22:50Z

I'm having the same bug on OSX, I'm going to take a closer look.

dresende · 2013-01-29T23:27:14Z

I think that the very first overflow number does not have the byte = 1 on index 7, it has byte = 255 on index 0-6.

kai-koch · 2013-01-29T23:28:50Z

I think BigNumber = require("bignumber.js"); should be moved to the top of the module as var declaration.
It will be cached before script execution.
This could also come in handy, if you should decide to use bignumbers.js else where in node-mysql.

dresende · 2013-01-29T23:33:54Z

OK, I'm just finishing bug fix and I'll update asap.

…754 precision limit

kai-koch · 2013-01-29T23:40:40Z

My Windows calculator says this:

72057594037927935
  FF FF FF FF FF FF FF
  1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111 1111

72057594037927936
1 00 00 00 00 00 00 00
1 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000 0000

But I don't know how it is encode in the buffer node-mysql is using.

dresende · 2013-01-29T23:47:27Z

It's exactly that, but reversed (little endian). But we're still doing it wrong. The upper limit must be 2^53, which is: 9007199254740992.

kai-koch · 2013-01-30T00:17:27Z

Works for both numbers now. :-)

dresende · 2013-01-30T00:20:56Z

@felixge : any objections? :)

kai-koch · 2013-01-30T00:35:25Z

Question: Is that a signed or unsigned Big Int encoded in the Buffer?

EDIT: Duh, unsigned of course, since it is defined by the table defenition

kai-koch · 2013-01-30T00:56:44Z

We still got a problem:
9223372036854775808, stored in DB, but displays as 9223372036854776000 in the example script.

(9223372036854775807 is the max. value for signed 64-Interger)

felixge · 2013-01-30T07:43:51Z

lib/protocol/Parser.js

@@ -154,30 +155,35 @@ Parser.prototype.parseLengthCodedNumber = function() {
  }

  var length;
+  var big_number = false;


The rest of the code would use bigNumber, not big_number.

felixge · 2013-01-30T07:47:29Z

@felixge : any objections? :)

Well, I dislike switching the returned type from Number to String on >= 53 bit, this could certainly introduce subtle bugs in clients. That being said, I can't think of a better solution either (other than switching to a more enlightened language like Go, which I did recently : ).

Anyway, patch LGTM otherwise.

dresende · 2013-01-30T09:37:09Z

We could make an option to enable (or disable) this. Just need to know what is best by default.

felixge · 2013-01-30T10:20:42Z

Just need to know what is best by default.

I would favor the current behavior as the default.

…r big numbers

sirian · 2013-01-30T11:27:25Z

yep. better to use current behavior as the default.
maybe enabling this option will also typecast bigint to string in select queries?
p.s. waiting fix)

…er number precision (if big number support is off)

dresende · 2013-01-30T11:32:40Z

Done some changes. What other behavior should be changed?

felixge · 2013-01-30T11:32:41Z

maybe enabling this option will also typecast bigint to string in select queries?

Yes.

p.s. waiting fix)

?

sirian · 2013-01-30T11:34:50Z

I'm waiting while it will be fixed in the npm repository
p.s. node-mysql-libmysqlclient also works incorrectly for bigint insert ids

…t cast to number if string value is over precision limit

dresende · 2013-01-30T11:44:08Z

For LONGLONG and other number columns, if supportBigNumbers option is enabled, won't be converted to Number if over precision limit.

…tion

kai-koch · 2013-01-30T18:51:06Z

I got the bignumber branch, it still has problems with numbers greater than '9223372036854775807'
line 170:

if (this._buffer[this._offset + 6].toString(2).length > 5 || this._buffer[this._offset + 7] == 1) {

Is wrong.
If I get the math right:

We have 8 Bytes that represent a 64-bit Number in LittleEndian (signed or unsigned? Does MySQL give this information?)
you then check:
- if in byte with index 6 one of the 3 highest bits are set. Which would mean, if any is set we have a number with more than 53 bits
- OR if bit 1 in in the byte with index 7 is set (bit 57 of the number).

This way you only detect big numbers that are smaller than 2^58.

So for unsigned 64-bit Integers line 156 must be:

if (this._buffer[this._offset + 6].toString(2).length > 5 || this._buffer[this._offset + 7]) {

Any bit set in byte with index 7 means BigNumber.

Which brings me to signed 64-Integers for these the line must look something like this:

if (this._buffer[this._offset + 6].toString(2).length > 5 ||
    (this._buffer[this._offset + 7] > 0 && this._buffer[this._offset + 7] < 128 ) ) {

Because:
if the highest bit in the byte with index 7 is set (bit 63) we have a negative number, which then again needs to be tested if it exceeds the 53 bits range of JS, depending on which bit-notation is given in the buffer by mysql for 64-bit signed .integers.

[edit: linenumber]

kai-koch · 2013-01-30T19:04:52Z

Another thing:
I think testing

this._buffer[this._offset + 6] > 31

is slightly faster than

this._buffer[this._offset + 6].toString(2).length > 5

kai-koch · 2013-01-30T19:17:23Z

While I am nagging, another suggestion since you are working on that part of the code anyway:
Change the name of variable 'byte' to 'bits'.
'bits' is no reserved word and makes clearer what the code does. :-P

dresende · 2013-01-30T19:27:46Z

I'm trusting on your code, I don't have time now to think about it :P I'm pulling the code now.

kai-koch · 2013-01-30T19:40:42Z

You better should have. ;)
The line for signed Integers breaks the current example code. :-(

Use the line for unsigend Integers, please:

if (this._buffer[this._offset + 6] > 31 || this._buffer[this._offset + 7]) {

kai-koch · 2013-01-30T19:45:35Z

This will work until someone works with signed 64-Bit greater than '9223372036854775807' or less than 0.

Changes parseLengthCodedNumber to parse big numbers

Add _binary prefix when interpolating []byte data.

dresende added 2 commits January 29, 2013 21:11

Use bignumber.js to process numbers over js precision range

9e5df78

This returns them as strings instead of numbers

Adds new dependency on bignumber.js

c623bea

dresende mentioned this pull request Jan 29, 2013

bigint insert ids error #380

Closed

felixge reviewed Jan 29, 2013
View reviewed changes

dresende added 2 commits January 29, 2013 23:03

Optimizes big number discovery, based on code from kai-koch

d5d8a0f

Adding small information about insert id returning string for big num…

d8569fe

…bers

Moves BigNumber to top of file, fixes bug for first number after ieee…

586554c

…754 precision limit

dresende added 2 commits January 30, 2013 00:04

Really fixes checking for big number

680aa2e

Updates parser test

c073bd8

felixge reviewed Jan 30, 2013
View reviewed changes

Changes big_number to bigNumber

221fa7f

dresende added 3 commits January 30, 2013 11:20

Add supportBigNumbers to ConnectionConfig (false by default)

f5da5d1

Pass config from Protocol to Parser

0b6fecb

Change Parser to look for supportBigNumbers option before checking fo…

f7a7378

…r big numbers

dresende added 3 commits January 30, 2013 11:28

Change Parser to have the previous behavior of throwing when value ov…

f5f9048

…er number precision (if big number support is off)

Revert Parser test

89ddb43

Changes Readme to mention supportBigNumbers option

09796a0

Changes RowDataPacket to check for big number support option and don'…

a413c6a

…t cast to number if string value is over precision limit

Add one more paragraph about precision limit and supportBigNumbers op…

1ea39e6

…tion

Makes a few changes provided by @kai-koch

6ec07eb

More changes... :)

04f2ab5

dresende added a commit that referenced this pull request Jan 31, 2013

Merge pull request #382 from felixge/bignumber

913fcbc

Changes parseLengthCodedNumber to parse big numbers

dresende merged commit 913fcbc into master Jan 31, 2013

dresende deleted the bignumber branch January 31, 2013 00:33

tomasikp mentioned this pull request Apr 2, 2013

Add bigNumberStrings option for forcing BIGINTs as strings (always) #437

Closed

dveeden pushed a commit to dveeden/mysql that referenced this pull request Jan 31, 2023

Merge pull request mysqljs#382 from ymmt2005/interpolate_binary

f368b3b

Add _binary prefix when interpolating []byte data.

Changes parseLengthCodedNumber to parse big numbers #382

Changes parseLengthCodedNumber to parse big numbers #382

Uh oh!

Conversation

dresende commented Jan 29, 2013

Uh oh!

dresende commented Jan 29, 2013

Uh oh!

kai-koch commented Jan 29, 2013

Uh oh!

kai-koch commented Jan 29, 2013

Uh oh!

felixge Jan 29, 2013

Choose a reason for hiding this comment

Uh oh!

dougwilson Jan 29, 2013

Choose a reason for hiding this comment

Uh oh!

kai-koch commented Jan 29, 2013

Uh oh!

dresende commented Jan 29, 2013

Uh oh!

kai-koch commented Jan 29, 2013

Uh oh!

dresende commented Jan 29, 2013

Uh oh!

dresende commented Jan 29, 2013

Uh oh!

kai-koch commented Jan 29, 2013

Uh oh!

dresende commented Jan 29, 2013

Uh oh!

kai-koch commented Jan 29, 2013

Uh oh!

dresende commented Jan 29, 2013

Uh oh!

kai-koch commented Jan 30, 2013

Uh oh!

dresende commented Jan 30, 2013

Uh oh!

kai-koch commented Jan 30, 2013

Uh oh!

kai-koch commented Jan 30, 2013

Uh oh!

felixge Jan 30, 2013

Choose a reason for hiding this comment

Uh oh!

felixge commented Jan 30, 2013

Uh oh!

dresende commented Jan 30, 2013

Uh oh!

felixge commented Jan 30, 2013

Uh oh!

sirian commented Jan 30, 2013

Uh oh!

dresende commented Jan 30, 2013

Uh oh!

felixge commented Jan 30, 2013

Uh oh!

sirian commented Jan 30, 2013

Uh oh!

dresende commented Jan 30, 2013

Uh oh!

kai-koch commented Jan 30, 2013

Uh oh!

kai-koch commented Jan 30, 2013

Uh oh!

kai-koch commented Jan 30, 2013

Uh oh!

dresende commented Jan 30, 2013

Uh oh!

kai-koch commented Jan 30, 2013

Uh oh!

kai-koch commented Jan 30, 2013

Uh oh!

Uh oh!